Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Windows support #50

Merged
merged 7 commits into from
Apr 16, 2024
Merged

Windows support #50

merged 7 commits into from
Apr 16, 2024

Conversation

deivid-rodriguez
Copy link
Contributor

We've been running turbo_tests in RubyGems with a monkeypatch to make Windows support work. A unified solution would be better but I guess getting things at least working should take priority so maybe there's interest in the raw patch.

Windows tests don't really catch any issues (they pass without this patch), although turbo_tests does not really work on Windows. In any case, as a minimal improvement over what we have, I added Windows to CI.

Finally, I dropped support for Ruby 2.7 since it reached its End of Life and also started testing with Ruby 3.3.

Happy to extract any of these changes to separate PRs!

ilyazub
ilyazub requested changes Apr 8, 2024
View reviewed changes
Copy link
Collaborator

@ilyazub ilyazub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding Windows to the CI. We can't drop Ruby 2.7 support, unfortunately.

I'm sorry, I didn't replace the mkfifo yet (ref: #38 (comment)).

turbo_tests.gemspec Outdated Show resolved Hide resolved
@deivid-rodriguez
Copy link
Contributor Author

I see, let me remove the commit dropping support for Ruby 2.7 then!

ilyazub
ilyazub requested changes Apr 10, 2024
View reviewed changes
Copy link
Collaborator

@ilyazub ilyazub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @deivid-rodriguez!

Let's just replace FIFO in lib/turbo_tests/runner.rb with RSPEC_FORMATTER_OUTPUT_ID for all platforms. (I'm working on this now, but feel free to give it a try too. :-))

lib/turbo_tests/windows.rb Outdated Show resolved Hide resolved
lib/turbo_tests/windows.rb Outdated
env["RSPEC_FORMATTER_OUTPUT_ID"] = SecureRandom.uuid
env["RUBYOPT"] = ["-I#{File.expand_path("..", __dir__)}", ENV["RUBYOPT"]].compact.join(" ")

command_name = Gem.win_platform? ? [Gem.ruby, "bin/rspec"] : "bin/rspec"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the best way to detect Bundler binstubs and/or if bundle exec is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point too! Using bin/rspec directly is quite specific to our environment. I guess we could detect whether there's a bin/rspec file relative to the cwd, and otherwise use bundle exec rspec?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could detect whether there's a bin/rspec file relative to the cwd, and otherwise use bundle exec rspec

Will Windows correctly handle this if we append $PWD/bin to ENV["PATH"] instead of detecting bin/rspec? (I'm new to Bundler internals. Below is a hacky pseudo-code.)

env["PATH"] << ":#{File.expand_path("bin", __dir__)}"

command_name = ENV["BUNDLE_BIN_PATH"] ? [ENV["BUNDLE_BIN_PATH"], "exec", "rspec"] : "rspec"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would handle it fine, but I'm not sure it's a good idea since it would have global side effects. Users have other scripts in bin/ and maybe won't expect turbo_tests to automatically set things up so that those other scripts start getting selected when their tests run processes? Not sure.

Maybe we can start leaving things as they are regarding building the command name for now. I could configure $LOAD_PATH on our side before launching turbo_tests so that our binstub gets properly loaded.

lib/turbo_tests/windows.rb Outdated Show resolved Hide resolved
ilyazub
ilyazub approved these changes Apr 10, 2024
View reviewed changes
Copy link
Collaborator

@ilyazub ilyazub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I unified start_subprocess code for all platforms in b4ad8f4 and resolved conflicts with the master branch in 8af698e.

@deivid-rodriguez Please let me know if these changes work for you and look good to you.

@deivid-rodriguez
Copy link
Contributor Author

Thanks for that! I will give this patch a try and confirm it works for us!

@deivid-rodriguez
Copy link
Contributor Author

I tried this branch through rubygems/rubygems@a4e3012 and Windows CI is cool with it.

I also tried it locally on Linux and found no issues either, so this seems good!

Unfortunately we're having an unrelated issue, and we had to pin to 2.2.0 for now, but this seems good to me regardless!

@ilyazub
Copy link
Collaborator

ilyazub commented Apr 15, 2024

Sounds good, thank you @deivid-rodriguez.

I'll merge this PR and will also investigate ruby/ruby#10496.

@javierjulio
Copy link
Contributor

@deivid-rodriguez @ilyazub I created #53 to resolve the OpenStruct issue (from #49) by replacing it with a Struct instead. I believe with that change, the json gem dependency could be removed since it was added in #49 along with OpenStruct.

ilyazub
ilyazub approved these changes Apr 16, 2024
View reviewed changes
Copy link
Collaborator

@ilyazub ilyazub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged origin/master to this branch to remove the ostruct gem dependency.

@ilyazub ilyazub merged commit 876a33d into serpapi:master Apr 16, 2024
10 of 11 checks passed
@deivid-rodriguez deivid-rodriguez deleted the windows-support branch April 16, 2024 14:55
@deivid-rodriguez
Copy link
Contributor Author

Thank you! Hopefully we'll be able to upgrade and remove our monkeypatches soon!

@ilyazub
Copy link
Collaborator

ilyazub commented Apr 16, 2024

I reverted #49 in #54 and am preparing the PR to fix the Ruby-core tests failure in commands/pristine_spec.rb.

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Apr 16, 2024 •
edited
Loading

Yeah, that should workaround the issue for us, thank you!

In any case, I think the ruby-core issue reveals some problem in bundler or ruby-core test setup, and I believe it's better to explicitly list dependencies that these days are maintained as separate gems.

This change is good for us since it unblock us from upgrading but if I manage to figure out the problem, I'll make sure to get back here and try to add back the explicit json dependency.

@ilyazub
Copy link
Collaborator

ilyazub commented Apr 16, 2024

@deivid-rodriguez bundle pristine failed in the Ruby-core tests (ref: ruby/ruby#10496 (comment))

Cannot pristine baz (1.0.0). Gem is sourced from local path.
The installation path is insecure. Bundler cannot continue.
/omitted/path/ruby/tmp/1/gems/system/extensions/x86_64-linux/3.4.0+0-static is world-writable (without sticky bit).
Bundler cannot safely replace gems in world-writeable directories due to potential vulnerabilities.
Please change the permissions of this directory or choose a different install path.

What does the The installation path is insecure error means?

@deivid-rodriguez
Copy link
Contributor Author

We try to explain it below. Basically means that the previous installation path has insecure world-writable permissions, and since Bundler needs to delete it for bundle pristine, if a malicious user, for example, symlinked a file in that folder to some other important file, Bundler could end up deleting it.

That all said, I don't know how or why the error is being triggered here 😮.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilyazub ilyazub ilyazub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@deivid-rodriguez @ilyazub @javierjulio